Skip to content

Add menu validate command to validate the format of menu files. #363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 17, 2025
Merged

Add menu validate command to validate the format of menu files. #363

merged 1 commit into from
Apr 17, 2025

Conversation

@dgarros
Copy link
Contributor

@dgarros dgarros commented Apr 16, 2025

and a few fixes and cleanup following the changes on object load command

@github-actions github-actions bot added the type/documentation Improvements or additions to documentation label Apr 16, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 16, 2025 •
edited
Loading

Deploying infrahub-sdk-python with  Cloudflare Pages  Cloudflare Pages

Latest commit: 247d209
Status: ✅  Deploy successful!
Preview URL: https://deb29408.infrahub-sdk-python.pages.dev
Branch Preview URL: https://dga-20250416-fix-menu-cmd.infrahub-sdk-python.pages.dev

View logs

@codecov
Copy link

codecov bot commented Apr 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.51724% with 20 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/ctl/menu.py 54.28% 16 Missing ⚠️
infrahub_sdk/spec/object.py 80.00% 2 Missing and 2 partials ⚠️ 
@@             Coverage Diff             @@
##           develop     #363      +/-   ##
===========================================
+ Coverage    73.77%   73.91%   +0.14%     
===========================================
  Files           92       92              
  Lines         8498     8536      +38     
  Branches      1664     1676      +12     
===========================================
+ Hits          6269     6309      +40     
+ Misses        1791     1781      -10     
- Partials       438      446       +8
Flag Coverage Δ
integration-tests 25.43% <32.75%> (+0.28%) ⬆️
python-3.10 47.36% <51.72%> (+0.41%) ⬆️
python-3.11 47.34% <51.72%> (+0.38%) ⬆️
python-3.12 47.34% <51.72%> (+0.38%) ⬆️
python-3.13 47.36% <51.72%> (+0.43%) ⬆️
python-3.9 45.75% <51.72%> (+0.39%) ⬆️
python-filler-3.12 24.26% <0.00%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/spec/menu.py 79.16% <100.00%> (+29.16%) ⬆️
infrahub_sdk/spec/object.py 84.78% <80.00%> (-0.77%) ⬇️
infrahub_sdk/ctl/menu.py 67.85% <54.28%> (+9.79%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dgarros dgarros requested a review from a team April 16, 2025 12:29
@dgarros dgarros marked this pull request as ready for review April 16, 2025 12:29
ogenstad
ogenstad approved these changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I'd need some clafification regarding the MenuFile class.

infrahub_sdk/spec/menu.py

def validate_content(self) -> None:
super().validate_content()
InfrahubFile.validate_content(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making this change? Swapping from inheriting from InfrahubFile and moving to ObjectFile instead but we override the spec method from ObjectFile completely and then call the .validate_content on InfrahubFile I assume to avoid using the one from ObjectFile. Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this isn't ideal, and it calls for a larger redesign / cleaned of the ObjectFile / YamlFile / MenuFile but I think this is a larger effort and I would prefer to manage that separately
I will open a dedicated issue to track that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#367

infrahub_sdk/ctl/menu.py

@app.command()
@catch_exception(console=console)
async def validate(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice with a unit test even if it's just for the failure scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I added some unit tests

@dgarros
Ensure default kind is properly propagated and that peer schema can't…
247d209 
… be a generic

Add `menu validate` command to validate the format of menu files.
@dgarros dgarros force-pushed the dga-20250416-fix-menu-cmd branch from f53022d to 247d209 Compare April 17, 2025 08:14
@dgarros dgarros merged commit cdf9717 into develop Apr 17, 2025
18 checks passed
@dgarros dgarros deleted the dga-20250416-fix-menu-cmd branch April 17, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogenstad ogenstad ogenstad approved these changes

Assignees

No one assigned

Labels

type/documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dgarros @ogenstad